samples: http_server: Fix warning about integer name #93468
Conversation
decsny
added
the
Trivial
zephyrbot
added
area: Sockets
zephyrbot
requested review from
jukkar,
kartben,
mrodgers-witekio,
nashif and
rlubos
| char name[MAX_NAME_LEN]; | ||
|
|
||
| snprintk(name, sizeof(name), "ws[%d]", slot); | ||
| snprintk(name, sizeof(name), "ws[%d]", slot % (10 * MAX_NAME_LEN)); |
There was a problem hiding this comment.
This does not look right. The idea is to print the slot value here. If the name variable is too small, let's increase the size of it instead, so something like
MAX_NAME_LEN sizeof("ws[xxxx]")
There was a problem hiding this comment.
yeah this is not right what I have here, but I think what you suggested also won't help, the issue appears to be that the compiler knows that the max integer value is 10 digits, so the max size would theoretically be sizeof("ws[xxxxxxxxxx]"), is that what you think it should be changed to ?
There was a problem hiding this comment.
I pushed the same idea as you said but did sizeof("ws[]") + 10
There was a problem hiding this comment.
Yes, this sizeof("ws[xxxxxxxxxx]") is what I was suggesting, so just adding enough x to silence the warning.
There was a problem hiding this comment.
Having +10 certainly works, but it is now a magical constant and not immediately obvious why it is there. Therefore we used the "template" way in other part of the code and I suggest we do that here too.
decsny
force-pushed
the
fix/http_server_ws_name_warning
branch
from
d81befc to
1e995c6
Compare
When building with CONFIG_DEBUG=y, there is a build warning about the name being truncated due to integer size is larger than MAX_NAME_LEN. So fix MAX_NAME_LEN to be large enough to handle 10 digit integer, which is maximum. Signed-off-by: Declan Snyder <[email protected]>
decsny
force-pushed
the
fix/http_server_ws_name_warning
branch
from
1e995c6 to
0c58043
Compare
|
|
|
||
| if (IS_ENABLED(CONFIG_THREAD_NAME)) { | ||
| #define MAX_NAME_LEN sizeof("ws[xx]") | ||
| #define MAX_NAME_LEN (sizeof("ws[xxxxxxxxxx]")) |
There was a problem hiding this comment.
@decsny - I don't want to be too nit-picky, so this isn't blocking by any means, but I think MAX_NAME_LEN should probably be defined in terms of CONFIG_THREAD_MAX_NAME_LEN. If you can respin, sure, but otherwise, I think we can probably merge anyway.
Below, N is the minimum expected number of digits to use for slots
| #define MAX_NAME_LEN (sizeof("ws[xxxxxxxxxx]")) | |
| BUILD_ASSERT(CONFIG_THREAD_MAX_NAME_LEN > (sizeof("ws[]") + N), "CONFIG_THREAD_MAX_NAME_LEN is too small"); | |
| #define MAX_NAME_LEN (CONFIG_THREAD_MAX_NAME_LEN - sizeof("ws[]")) |
There was a problem hiding this comment.
defer to @jukkar who had this opinion for how it is now
There was a problem hiding this comment.
I suggest that we go with what we have now, we have used this pattern in other part of networking code too. If we want to have build asserts, we could introduce that to all places that have the name used.
|
|
||
| if (IS_ENABLED(CONFIG_THREAD_NAME)) { | ||
| #define MAX_NAME_LEN sizeof("ws[xx]") | ||
| #define MAX_NAME_LEN (sizeof("ws[xxxxxxxxxx]")) |
There was a problem hiding this comment.
I suggest that we go with what we have now, we have used this pattern in other part of networking code too. If we want to have build asserts, we could introduce that to all places that have the name used.
When building with CONFIG_DEBUG=y, there is a build warning about the name being truncated due to integer size is larger than MAX_NAME_LEN. So fix with modulo operator to be clear to the compiler what we expect.